Skip to content

Implement no class feature#316

Closed
pfazzi wants to merge 2 commits intomainfrom
no-class
Closed

Implement no class feature#316
pfazzi wants to merge 2 commits intomainfrom
no-class

Conversation

@pfazzi
Copy link
Copy Markdown
Collaborator

@pfazzi pfazzi commented Nov 22, 2022 •
edited
Loading

Closes #315 allowing to write rules like:

Rule::noClass()
  ->should(new ResideInTheseNamespaces('App\Services'))
  ->because('this namespace has been deprecated in favor of the modular architecture');

or

Rule::noClass()
  ->that(new ResideInOneOfTheseNamespaces('App\Entity'))
  ->should(new HaveNameMatching('*Service'))
  ->because('of our naming convention');

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 22, 2022 •
edited by codecov bot
Loading

Codecov Report

❌ Patch coverage is 92.85714% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.23%. Comparing base (392757f) to head (cd1fda2).
⚠️ Report is 486 commits behind head on main.

Files with missing lines Patch % Lines
src/Rules/NoClass.php 72.72% 3 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main     #316      +/-   ##
============================================
+ Coverage     93.02%   93.23%   +0.20%     
- Complexity      476      487      +11     
============================================
  Files            60       62       +2     
  Lines          1262     1301      +39     
============================================
+ Hits           1174     1213      +39     
  Misses           88       88

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pfazzi pfazzi marked this pull request as draft November 22, 2022 22:48
AlessandroMinoccheri
AlessandroMinoccheri reviewed Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@AlessandroMinoccheri AlessandroMinoccheri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to add a description of this new behavior into the README file

src/Rules/RuleBuilder.php
}

public function setRunOnlyThis(): self
public function negateShoulds(): self
Copy link
Copy Markdown
Member

@AlessandroMinoccheri AlessandroMinoccheri Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you do this change?

micheleorselli
micheleorselli reviewed Nov 23, 2022
View reviewed changes
src/Expression/NegateDecorator.php
use Arkitect\Rules\ViolationMessage;
use Arkitect\Rules\Violations;

class NegateDecorator implements Expression
Copy link
Copy Markdown
Member

@micheleorselli micheleorselli Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about naming it just Not? I could also simplify other expressions

Copy link
Copy Markdown
Collaborator Author

@pfazzi pfazzi Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree!

src/Rules/Rule.php
return new AllClasses();
}

public static function noClass(): NoClass
Copy link
Copy Markdown
Member

@micheleorselli micheleorselli Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about naming it NoClasses, for consistency with AllClasses?

Copy link
Copy Markdown
Collaborator Author

@pfazzi pfazzi Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it would not be correct English. It's the same in Italian "tutte le classi"/"nessuna classe".

Copy link
Copy Markdown
Member

@micheleorselli micheleorselli Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am far from being an expert but I think also NoClasses is correct. Looking at java's ArchUnit they decided to used it, eg.

ArchRule rule = ArchRuleDefinition.noClasses()
    .that().resideInAPackage("..service..")
    .should().accessClassesThat().resideInAPackage("..controller..");

rule.check(importedClasses);

src/Rules/RuleBuilder.php
{
if ($this->negateShoulds) {
$should = new NegateDecorator($should);
}
Copy link
Copy Markdown
Member

@micheleorselli micheleorselli Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am too low on ☕ but I can't wrap my head around this, would you mind explaining it to me?
If the should method in NoClass already does this

$this->ruleBuilder->addShould(new NegateDecorator($expression));

why do we need to add another new NegateDecorator here?

@fain182 fain182 added this to the v1 milestone Dec 14, 2022
@fain182 fain182 removed this from the v1 milestone Mar 4, 2026
@pfazzi pfazzi closed this Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@micheleorselli micheleorselli micheleorselli left review comments

@AlessandroMinoccheri AlessandroMinoccheri AlessandroMinoccheri left review comments

@fain182 fain182 Awaiting requested review from fain182

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

No classes should DSL

5 participants

@pfazzi @codecov-commenter @micheleorselli @AlessandroMinoccheri @fain182